-
Notifications
You must be signed in to change notification settings - Fork 349
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
commit_templater: encode elided node as Option<Commit>
#3319
Conversation
1f3d5f3
to
602f09e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how I feel about this. I like the idea of having optional values in general, but I I'm not convinced the jj log
template should have Option<Commit>
as context object (I'm not convinced they shouldn't either).
This is primarily designed for the graph node template. I thought it could apply to the text part, but I agree it wouldn't be that useful. Regarding the context object, I hope we can eventually relax it to any types known to the template language. Perhaps, |
Since impl TemplateLanguage for CommitTemplateLanguage {
type Context = ();
}
let placehodler = ..; // Rc<RefCell<C>> stuff to insert arbitrary "self" variable
let make_self = || some_wrapping(placeholder);
ctx.local_variables.insert("self", &make_self);
let inner_template: Box<Template<()>> = build(language, &ctx, node)?;
let outer_template: Template<C> = Wrapper { inner_template, placeholder } It's a bit messy, but seems doable. FWIW, do you like the idea of using |
I am mainly reacting to the title of the PR, so I could easily be missing the point, but I'd love for an elided node to eventually keep track of approximately how many commits were elided. Perhaps the type should be called something else, even if it's equivalent to |
Yes. It suggests that the "(elided revisions)" text shouldn't be a part of the common commit template. I still think templates.log_node = 'if(self, if(current_working_copy, "@", "o"), "=")' |
I think Option<Commit> is the simplest encoding of the log node. The behavior of an Option type is closer to nullable types rather than the Option in Rust. I don't think we would want to write opt.map(|x| x.f()) or opt.unwrap().f(). We can of course add opt?.f() syntax, but it will be a short for "if(opt, opt.f())"?
Since elided graph entry has no associated commits, it makes some sense to represent as None?
This is now ready for review.
|
Option<Commit>
(PoC)Option<Commit>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, eventually we would probably want to expose more things like ascii/non-ascii and at that point elided could maybe be a property. But until that point this simplifies things. |
For ascii/non-ascii, maybe we can add |
#3260
Checklist
If applicable:
CHANGELOG.md